Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a general approach to show tooltip, fix temporary tooltip bug #23574

Merged
merged 11 commits into from
Mar 23, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 19, 2023

TLDR

  • Improve performance: lazy creating the tippy instances.
  • Transparently support all "tooltip" elements, no need to call initTooltip again and again.
  • Fix a temporary tooltip re-entrance bug, which causes showing temp content forever.
  • Upgrade vue3-calendar-heatmap to 2.0.2 with lazy tippy init (initHeatmap time decreases from 100ms to 50ms)

Details

The performance

Creating a lot of tippy tooltip instances is expensive. This PR doesn't create all tippy tooltip instances, instead, it only adds "mouseover" event listener to necessary elements, and then switches to the tippy tooltip

The general approach for all tooltips

Before, dynamically generated tooltips need to be called with initTooltip.

After, use MutationObserver to:

  • Attach the event listeners to newly created tooltip elements, work for Vue (easier than before)
  • Catch changed attributes and update the tooltip content (better than before)

It does help a lot, eg:

<div class="ui label">
<!-- TODO: Convert to tooltip once we can use tooltips in Vue templates -->
{{ mergeForm.textClearMergeMessageHint }}
</div>

Temporary tooltip re-entrance bug

To reproduce, on try.gitea.io, click the "copy clone url" quickly, then the tooltip will be "Copied!" forever.

After this PR, with the help of attachTippyTooltip, the tooltip content could be reset to the default correctly.

Other changes

  • data-tooltip-content is preferred from now on, the old data-content may cause conflicts with other modules.
  • data-placement was only used for tooltip, so it's renamed to data-tooltip-placement, and removed from createTippy.

@silverwind
Copy link
Member

silverwind commented Mar 19, 2023

One change I was planning to to is to remove the tooltip class and have it based purely on a single data-tooltip="content" attribute. Would recommend if we touch tooltips deeply, we do this refactor here now.

I certainly can't approve two ways to init tooltips like this PR does via coexistance of data-tooltip-content and data-content. There should only be one way.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 19, 2023
@wxiaoguang
Copy link
Contributor Author

One change I was planning to to is to remove the tooltip class and have it based purely on a single data-tooltip="content" attribute. Would recommend if we touch tooltips deeply, we do this refactor here now.

It would be too much. Could we just have this one, and do the replacing data-content => data-tooltip-content in next PR?

@wxiaoguang
Copy link
Contributor Author

I certainly can't approve two ways to init tooltips like this PR does via coexistance of data-tooltip-content and data-content. There should only be one way.

## Refactoring Suggestions
* Design more about the future, but not only resolve the current problem.
* Reduce ambiguity, reduce conflicts, improve maintainability.
* Describe the refactoring, for example:
* Why the refactoring is needed.
* How the legacy problems would be solved.
* What's the Pros/Cons of the refactoring.
* Only do necessary changes, keep the old logic as much as possible.
* Introduce some intermediate steps to make the refactoring easier to review, a complete refactoring plan could be done in several PRs.
* If there is any divergence, the TOC(Technical Oversight Committee) should be involved to help to make decisions.
* Add necessary tests to make sure the refactoring is correct.
* Non-bug refactoring is preferred to be done at the beginning of a milestone, it would be easier to find problems before the release.
## Reviewing & Merging Suggestions
* A refactoring PR shouldn't be kept open for a long time (usually 7 days), it should be reviewed as soon as possible.
* A refactoring PR should be merged as soon as possible, it should not be blocked by other PRs.
* If there is no objection from TOC, a refactoring PR could be merged after 7 days with one core member's approval (not the author).
* Tolerate some dirty/hacky intermediate steps if the final result is good.
* Tolerate some regression bugs if the refactoring is necessary, fix bugs as soon as possible.

  • Introduce some intermediate steps to make the refactoring easier to review, a complete refactoring plan could be done in several PRs.
  • Tolerate some dirty/hacky intermediate steps if the final result is good.

@silverwind
Copy link
Member

data-tooltip-content

Please just use data-tooltip, less to write and same effect.

Tolerate some dirty/hacky intermediate steps if the final result is good.

I don't know about others but I generally can't accept suboptimal "intermediate" solutions. Just go all the way please.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 19, 2023

data-tooltip-content

Please just use data-tooltip, less to write and same effect.

I do like data-tooltip-content, it matches tippy.setContent. Maybe let most maintainers choose.

Tolerate some dirty/hacky intermediate steps if the final result is good.

I don't know about others but I generally can't accept suboptimal "intermediate" solutions. Just go all the way please.

If you do not have strong objection, I will ask other maintainers' opinions.

@silverwind
Copy link
Member

silverwind commented Mar 19, 2023

I don't know but I think I've personally had enough of your "temporary" solutions. I want robust, simple and easy to maintain solutions, not such a mess like the getComputedStyle when toggling a class.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 19, 2023

I don't know but I think I've personally had enough of your "temporary" solutions. I want robust, simple and easy to maintain solutions, not such a mess like the getComputedStyle when toggling a class.

I do not know what do you mean.

I have done a lot of refactorings, all of them are stable. And I have used my solutions (which you don't like, for example, catch frontend bugs, improve async calls, use pageData to introduce more Vue componenets, etc) resolved many Gitea bugs.

For the getComputedStyle, it doesn't affect anything, why it would bother you? Actually, you forced to make the show/hide refactoring in one PR, then some regression bugs come. If we could do it step by step, I am sure many regression bugs could be avoided. (Update: if you want to remove the assertion code, you can take a look at #23576)

I always finish my promises, to make the refactorings clear and stable. You can check:

I do not understand what you mean by "mess". If you say mess, the Gitea frontend (before I joint) was totally a mess.

if (mutation.type === 'childList') {
for (const el of mutation.addedNodes) {
// handle all "tooltip" elements in newly added nodes, skip non-related nodes (eg: "#text")
if (el.querySelectorAll) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line looks broken.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not broken, just a quick check to see the "node" has querySelectorAll function, which is needed by attachChildrenLazyTippyTooltip below.

Otherwise some nodes (like #text) couldn't be used to attach tippy.


I agree that there would be other approaches to check if the node is non-#text, at the moment, this quick check is clear and fast IMO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use node.nodeType please, it's much more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a list for the types?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be [Node.ELEMENT_NODE,Node.DOCUMENT_FRAGMENT_NODE].includes(el.nodeType).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that's slower than the if (el.querySelectorAll) check. Do you really want to do so?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively 'querySelectorAll' in el would also make the indent clear enough for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, then 'querySelectorAll' in el : ba8ad89

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@silverwind
Copy link
Member

silverwind commented Mar 19, 2023

I can accept a few of these changes here but the duality of data-content and data-tooltip-content needs to go. There must only be one attribute. Wich name it is, we can bikeshed later.

@HesterG
Copy link
Contributor

HesterG commented Mar 20, 2023

DiffFileTreeItem might need :data-content="item.name" as it is also a tooltip?

@wxiaoguang
Copy link
Contributor Author

DiffFileTreeItem might need :data-content="item.name" as it is also a tooltip?

According the comment below that line, the title is used by design. I have reviewed the DiffFileTreeItem PR before, at that time, most people agree the the "tippy tooltip" is too heavy, so the DiffFileTreeItem only used the title. I believe the "tooltip" class on it could be removed (I will remove it in this PR)

@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2023

Codecov Report

Merging #23574 (690ed36) into main (f521e88) will increase coverage by 0.01%.
The diff coverage is 37.89%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main   #23574      +/-   ##
==========================================
+ Coverage   47.14%   47.16%   +0.01%     
==========================================
  Files        1149     1154       +5     
  Lines      151446   152353     +907     
==========================================
+ Hits        71397    71854     +457     
- Misses      71611    72014     +403     
- Partials     8438     8485      +47     
Impacted Files Coverage Δ
cmd/dump.go 0.67% <0.00%> (ø)
cmd/web.go 0.00% <0.00%> (ø)
models/actions/run.go 1.64% <0.00%> (-0.08%) ⬇️
models/actions/runner.go 1.44% <ø> (ø)
models/packages/package.go 45.45% <0.00%> (-1.13%) ⬇️
models/user/search.go 77.50% <0.00%> (-6.29%) ⬇️
modules/actions/github.go 0.00% <0.00%> (ø)
modules/actions/workflows.go 0.00% <0.00%> (ø)
modules/context/context.go 64.54% <0.00%> (-3.53%) ⬇️
modules/doctor/storage.go 31.93% <0.00%> (ø)
... and 33 more

... and 34 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@silverwind silverwind changed the title Use a general apporach to show tooltip, fix temporary tooltip bug Use a general approach to show tooltip, fix temporary tooltip bug Mar 20, 2023
@lunny lunny added the topic/ui Change the appearance of the Gitea UI label Mar 21, 2023
@lunny lunny added this to the 1.20.0 milestone Mar 21, 2023
@wxiaoguang wxiaoguang mentioned this pull request Mar 21, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 23, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 23, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 23, 2023
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 23, 2023

but the duality of data-content and data-tooltip-content needs to go.

There are still many new PRs still using data-content. To avoid conflicts and make things clear, I will fix it in next PR.


To explain why (with details):

If I also refactor data-content, then this PR will get conflicts when other PRs come, I am sure nobody wants to review the conflicts again and again, which are mixed with a lot of other code.

To make things clear, in next PR, we "only" rename the data-content, even if some conflicts come, it's still clear that we can focus on the "name" itself, no need to care about other code.


Please follow #23649 after this gets merged.

@lunny lunny merged commit 9be90a5 into go-gitea:main Mar 23, 2023
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 23, 2023
@wxiaoguang wxiaoguang deleted the fix-tippy-tooltip branch March 23, 2023 09:56
@wxiaoguang
Copy link
Contributor Author

but the duality of data-content and data-tooltip-content needs to go.

Let's move to #23649 , it's big enough for reviewing.

zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 24, 2023
* upstream/main:
  Fix incorrect `HookEventType` of pull request review comments (go-gitea#23650)
  [skip ci] Updated translations via Crowdin
  Fix codeblocks in the cheat sheet (go-gitea#23664)
  Drop migration for ForeignReference (go-gitea#23605)
  Fix new issue/pull request btn margin when it is next to sort (go-gitea#23647)
  A tool to help to backport locales, changes source strings to fix other broken translations (go-gitea#23633)
  Fix incorrect `show-modal` and `show-panel` class (go-gitea#23660)
  Restructure documentation. Now the documentation has installation, administration, usage, development, contributing the 5 main parts (go-gitea#23629)
  Check LFS/Packages settings in dump and doctor command (go-gitea#23631)
  Use a general approach to show tooltip, fix temporary tooltip bug (go-gitea#23574)
  Improve workflow event triggers (go-gitea#23613)
  Improve `<SvgIcon>` to make it output `svg` node and optimize performance (go-gitea#23570)
lunny pushed a commit that referenced this pull request Mar 24, 2023
Follow:
* #23574
* Remove all ".tooltip[data-content=...]"

Major changes:

* Remove "tooltip" class, use "[data-tooltip-content=...]" instead of
".tooltip[data-content=...]"
* Remove legacy `data-position`, it's dead code since last Fomantic
Tooltip -> Tippy Tooltip refactoring
* Rename reaction attribute from `data-content` to
`data-reaction-content`
* Add comments for some `data-content`: `{{/* used by the form */}}`
* Remove empty "ui" class
* Use "text color" for SVG icons (a few)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants